Skip to content

fix: use Contains instead of HasPrefix for delivery repo detection#381

Merged
qiuzhiqian merged 1 commit intodevelop/intranet-updatefrom
fix-test-err
Apr 18, 2026
Merged

fix: use Contains instead of HasPrefix for delivery repo detection#381
qiuzhiqian merged 1 commit intodevelop/intranet-updatefrom
fix-test-err

Conversation

@qiuzhiqian
Copy link
Copy Markdown

The delivery:// scheme may not always appear at the beginning of the source string, so using Contains is more robust for detecting delivery repositories.

The delivery:// scheme may not always appear at the beginning of the
source string, so using Contains is more robust for detecting delivery
repositories.
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 18, 2026

CLA Assistant Lite bot:
提交邮箱中包含我们的合作伙伴,但您似乎并非合作伙伴的成员或对接人,请联系相关对接人将您添加至组织之中,或由其重新发起 Pull Request。
The commit email domain belongs to one of our partners, but it seems you are not yet a member of the current organization, please contact the contact person to add you to the organization or let them submit the Pull Request.

xml seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You can retrigger this bot by commenting recheck in this Pull Request

@deepin-ci-robot
Copy link
Copy Markdown

deepin pr auto review

针对这段 git diff 代码审查,我发现了一个严重的逻辑回归问题,同时也对代码质量提出建议。以下是详细的审查意见:

1. 逻辑与功能审查 (严重)

  • 问题点:将 strings.HasPrefix 修改为 strings.Contains 会导致逻辑错误,破坏了原有的业务语义。
  • 分析
    • 原代码 (HasPrefix):检查 repo.Source 是否 "delivery://" 开头。这符合 URL 或 URI 的标准校验逻辑,即判断源地址是否来自 delivery 协议。
    • 新代码 (Contains):检查 repo.Source 是否包含 "delivery://"
  • 潜在风险:如果 repo.Source 是一个复杂的字符串,例如 "http://example.com/mirror/delivery://repo" 或者文件路径中恰好包含该字符串,Contains 会错误地返回 true,导致程序误判该仓库为交付仓库,从而引发后续处理流程的错误。
  • 建议强烈建议回退此修改,保持使用 strings.HasPrefix。除非业务需求明确指出需要匹配字符串中间的内容,否则对于协议头、前缀的判断必须使用 HasPrefix

2. 代码质量与规范

  • 代码可读性:当前的逻辑判断非常直接,无论是 HasPrefix 还是 Contains,代码本身都很清晰。但在修复逻辑回归后,建议添加注释说明为何要检查该前缀,例如:
    // 检查是否为交付仓库源(delivery协议)
    if strings.HasPrefix(repo.Source, "delivery://") {
        return true
    }

3. 性能分析

  • 性能对比
    • strings.HasPrefixstrings.Contains 在底层实现上都是基于内存字节比较。
    • 在大多数情况下,HasPrefix 只需要比较字符串开头的几个字节,一旦不匹配立即返回。
    • Contains 需要遍历整个字符串(直到找到匹配项或到达末尾)。
  • 结论:从性能角度看,HasPrefix 通常优于 Contains(尤其是当字符串很长且匹配项在末尾或不存在时)。因此,即使不考虑逻辑错误,为了性能也应保留 HasPrefix

4. 安全性审查

  • 输入验证:代码中直接访问 repo.Source。如果 repo.Source 可能为 nil(虽然 Go 中 string 不会是 nil,但可能是空字符串),strings 包的函数能安全处理空字符串,不会引发 Panic。
  • 注入风险:此处仅做字符串匹配,不涉及命令执行或 SQL 拼接,暂无明显安全风险。
  • 注意:如果 repo.Source 的内容来源于不可信的用户输入,且后续用于构建系统命令或文件路径,需要严格校验。但在此片段中仅用于布尔判断,风险较低。

总结与改进建议

结论:此修改引入了逻辑 Bug,降低了代码性能,必须修复

建议修改代码(恢复原逻辑并添加注释):

func (m *UpdatePlatformManager) HasDeliveryRepo() bool {
    for _, repo := range m.repoInfos {
        // 使用 HasPrefix 确保只匹配以 delivery:// 开头的源地址,避免误匹配路径中包含该字符串的情况
        if strings.HasPrefix(repo.Source, "delivery://") {
            return true
        }
    }
    return false
}

@deepin-ci-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: electricface, qiuzhiqian

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@qiuzhiqian qiuzhiqian merged commit 8a83973 into develop/intranet-update Apr 18, 2026
22 of 29 checks passed
@qiuzhiqian qiuzhiqian deleted the fix-test-err branch April 18, 2026 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants